Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Debug Forkchoice endpoint support #49

Merged
merged 13 commits into from
Jul 20, 2023
Merged

Conversation

samcm
Copy link
Contributor

@samcm samcm commented Mar 20, 2023

Adds support for the new ForkChoice debug endpoint.

I wasn't entirely sure on the structure/formatting of where to place these structs. Happy to be advised if there is a better way to do things 🙏

Thank you!

@mcdee
Copy link
Contributor

mcdee commented Mar 22, 2023

This endpoint doesn't seem to be very well supported:

  • lighthouse: no support
  • nimbus: returns data format not as per the spec
  • prysm: no support
  • teku: support

so 1/4 of the nodes on which I tried it. I'm a little concerned about adding support for this in to the codebase and then being pinged on issues using it due to the lack of downstream support. Are there plans for the various clients to add/fix support?

@samcm
Copy link
Contributor Author

samcm commented Mar 22, 2023

Yeah that's one of my concerns as well. Maybe it makes sense to rename the func to DebugForkChoice or something. As for support, yeah it's in flight with multiple clients:

Teku: Implemented
Lighthouse: Open PR sigp/lighthouse#4003
Prysm: In develop - prysmaticlabs/prysm@3d0ecdf
Nimbus: Behind --debug-fork-choice (I haven't validated the data though, you could definitely be correct that the format isn't per the spec)
Lodestar: Unfinalized implementation running on /eth/v0/debug/forkchoice

@samcm
Copy link
Contributor Author

samcm commented Mar 22, 2023

Happy for this to stay unmerged until more support lands in client releases @mcdee FWIW 🙏

@mcdee
Copy link
Contributor

mcdee commented Mar 24, 2023

Yeah let's hold this until we see better coverage of support and can take a view on where it fits.

@samcm
Copy link
Contributor Author

samcm commented May 2, 2023

@mcdee All clients excluding Lodestar have support in at least their develop branches now 🙏

Copy link
Contributor

@mcdee mcdee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One change required. Also, please could you add this call to the sleepy and erroring clients in the testclients directory? Thanks.

api/v1/forkchoice.go Outdated Show resolved Hide resolved
@mcdee
Copy link
Contributor

mcdee commented Jul 2, 2023

There are a few errors being thrown up by the testing, please could you rebase this against he latest master and we'll see what needs to be addressed? Thanks.

@mcdee
Copy link
Contributor

mcdee commented Jul 20, 2023

Not sure exactly where those errors are (the output of the testing looks bad), I'll merge this and fix if required in a separate commit.

@mcdee mcdee merged commit 6dbe2ec into attestantio:master Jul 20, 2023
0 of 4 checks passed
@samcm
Copy link
Contributor Author

samcm commented Jul 31, 2023

Cheers @mcdee! Sorry I've been on holidays for a little bit, thank you for sorting that 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants